Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DO NOT MERGE] Post Election Changes #1731

Closed
wants to merge 21 commits into from
Closed

Conversation

huwd
Copy link
Member

@huwd huwd commented Nov 8, 2019

Trello: https://trello.com/c/uLLaEVPV/186-dont-deploy-update-checker-results-page

PEP restricted PR

🚨 Do not merge until product has approved. 🚨

Included Changes:

@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 8, 2019 16:36 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 8, 2019 16:39 Inactive
@huwd huwd changed the title [DO NOT MERGE] Add citizen groupings to results page [DO NOT MERGE] Add Subgroups to the results page Nov 8, 2019
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 8, 2019 17:30 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 11, 2019 13:43 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 11, 2019 15:11 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 11, 2019 15:26 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 11, 2019 15:59 Inactive
@bevanloon bevanloon temporarily deployed to finder-frontend-pr-1731 November 12, 2019 12:10 Inactive
Copy link
Contributor

@koetsier koetsier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good stuff. Few points:

  • Maybe split the helper up - it is a lot of complex code to get through.
  • I'm not sure about combining the actions and groups like this

@vanitabarrett
Copy link
Contributor

vanitabarrett commented Nov 13, 2019

The spacing above criteria seems quite large:

Screen Shot 2019-11-13 at 09 53 35

and spacing between the end of criteria and the start of actions seems a bit too small:

Screen Shot 2019-11-13 at 09 55 30

@vanitabarrett
Copy link
Contributor

Is there meant to be a border-top on the email subscription section?

Screen Shot 2019-11-13 at 09 58 12

@vanitabarrett
Copy link
Contributor

Sidebar layout:

There's an additional wrapping govuk-grid-column-one-third which makes it appear too small

Screen Shot 2019-11-20 at 11 36 20

huwd and others added 18 commits November 27, 2019 16:50
Tests fail if it's a <p> tag instead of <div>, view layers remain unchanged when it is either
Updated to match newer choice of assistance pet, avoid travel business question
There's only a single test here anyway, so it doesn't dry out much, and there's an irregular test failure reporting 'No such file or directory @ rb_sysopen - /tmp/downloaded.csv20191111-37-uz5lkw', this is an attempt to resolve any ordering issues
This is a bold title above the link.
We move this CSS out of the .app-c-email-link__link hierarchy as it's no longer
a link, and we remove changing the colour of the icon to match the link colour.
Otherwise the blue border sits against the grey border of the 'stay_updated'
section.
@huwd huwd changed the title [DO NOT MERGE] Add Subgroups to the results page [DO NOT MERGE] Post Election Changes Nov 27, 2019
@huwd huwd closed this Dec 9, 2019
@barrucadu barrucadu deleted the groupActionsByResults branch December 8, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants